[19.0][IMP] fs_attachments: fix parsing of stored filenames when storing locally - #638

Hi @lmignon,
some modules you are maintaining are being modified, check this out!

@CRogos Can you explain your use case? I can't imagine the context in which you're encountering this problem. How can I reproduce your issue?

The error occurred when trying to post an expense in our staging system.

Because we don't want that any attachment of the production system is deleted by accident from the staging server, we replace the read/write credentials with with readonly credentials by an neutralize.sql script. We also deactivate that new attachments are uploaded to the cloud storage from the staging system, by removing all models from the fs.storage setting. Instead they are stored the usual odoo.sh way on the server. All attachments which were copied from the production system are still accessible by the readonly connection.

UPDATE fs_storage
      SET server_env_defaults = REPLACE(
          (
              SELECT value
                FROM ir_config_parameter
                WHERE key = 'c4a8_setting.fs_storage_env_azure_readonly'
          ),
          '"x_directory_path_env_default": ""',
          CONCAT('"x_directory_path_env_default": "', code, '"')
      )
      WHERE code like 'azure-%';

So the error occurs when the module is installed, but the new attachment is not stored in the cloud instead on the native story.

@CRogos IMO, the issue you're experiencing comes from the way the library is being used and how your staging process is managed. We have never encountered this problem because our approach is different.

With your current approach, you can end up with records whose data is inconsistent with the storage configuration. If I understand your explanation correctly, you're mixing records from different storages that are associated with the same storage configuration.

A safer approach for a staging environment is to add a dedicated storage backend and configure it as the default for attachments (https://github.com/OCA/storage/tree/18.0/fs_attachment#tips--tricks). At the same time, you can configure the production storage to use a read-only user. This avoids inconsistencies while still allowing you to work safely with production data.

With your current approach, you can end up with records whose data is inconsistent with the storage configuration. If I understand your explanation correctly, you're mixing records from different storages that are associated with the same storage configuration.

No, I actually only disable all configurations from write access by removing all models from the configuration and changing the credentials with read only access.
As a result, no matching configuration is found for new files and therefore are written to the default, like you suggested but without an explicit configuration. So the only difference is that I do not create an explicit local file configuration, instead I use the default odoo behavior.

Could it be, that there is no "://" in the file path by odoo default, and therefore _fs_parse_store_fname is called without "://" in the url? I agree that when creating an explicit file default configuration for the staging system, the url would look something like this "local-file://..." and therefore the error should not occur, but I think the module should not crash, if the odoo native format is used, instead it should behave like odoo native.

In your stack_trace, the error occurs into the method compute of the fs_url. Nevertheless this compute only occurs if your attachment has a fs_filename and the fs_filename should only be filled if the attachment is stored into a configured fs_storage 🤔 ....

The method _fs_parse_store_fname should only be called if the attachment is linked to a fs_storage.... and in such a case the fname should contain "://" 🤔 🤔 🤔

This is my configuration. The only difference between prod/staging is client_id/_secret and on staging all model_ids are removed.

The place where the error occurs is into a record that should be linked to fs_storage


Can you check why you've a fs_filename with a fname that doesn't contain '://'?

I think this ir_attachment is not connected to any fs_storage.

I think this ir_attachment is not connected to any fs_storage.

In such a case, fs_filename should be False and the method not called.

I agree... I assume _compute_fs_url is called for new record.
Is fs_storage_id already set? Than we should change this method, so rec.fs_url is set to False if not rec.fs_storage_id

@CRogos The guard is if rec.fs_filename ... This field should never be set if no fs_folder_id.

Not sure if this is the case here, but we should also consider this scenario:
The attachment exists on fs_storage, than the configuration is changed and new content should be stored locally (no explicit configuration).
The storage configuration (for read) is still on the attachment record, when the new content is set and _compute_fs_url is called.

Not sure if this is the case here, but we should also consider this scenario: The attachment exists on fs_storage, than the configuration is changed and new content should be stored locally (no explicit configuration). The storage configuration (for read) is still on the attachment record, when the new content is set and _compute_fs_url is called.

In such a case, the attachment is linked to a storage. If the configuration is stored on the record, then the record is linked to a storage. But once again, we already have a guard with if rec.fs_filename. I'm sorry but without a proper understanding of the actual issue, the proposed fix may not be valid.